Skip to content

Conversation

@defaultdino
Copy link
Contributor

@defaultdino defaultdino commented Apr 13, 2025

This change requires using a private key file for Apple Sign-In authentication instead of directly pasting the key content.

  • Using a file is more reliable and secure
  • Copy-pasting key content can easily introduce formatting errors (extra spaces, line breaks, encoding issues)
  • From experience, copy-pasting the key did not even work properly, only until I changed this myself and built a new binary
  • Loading from a file ensures the key is parsed exactly as expected, reducing the chance of silent failures or invalid signature errors, as Apple does not even provide an error that this is what occurs unless you'd manually debug it like I did - verifying the issue is with the MAS binary.

Fixes #4391

@CLAassistant
Copy link

CLAassistant commented Apr 13, 2025

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@sandhose sandhose left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I don't know why the documentation was so far off the actual configuration 😅

@defaultdino
Copy link
Contributor Author

defaultdino commented Apr 14, 2025

Thanks! I don't know why the documentation was so far off the actual configuration 😅

No worries. I believe I have implemented your change requests now and I appreciate the quick review. Let me know if you don't agree with some changes I did

edit:

I have built the new resulting MAS cli using this branch and have deployed it fine in my cluster, and verified Apple SSO works.

@defaultdino defaultdino requested a review from sandhose April 14, 2025 10:10
@defaultdino
Copy link
Contributor Author

@sandhose I have now implemented the private key reading in the sync.rs file instead, and it will not break any current functionality - but if a private key file is given, it will be read. However, the raw private key takes precedence.

@defaultdino defaultdino requested a review from sandhose April 14, 2025 16:54
Copy link
Member

@sandhose sandhose left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this!

Comment on lines +192 to +203
} else if let Some(mut siwa) = provider.sign_in_with_apple.clone() {
// if private key file is defined and not private key (raw), we populate the
// private key to hold the content of the private key file.
// private key (raw) takes precedence so both can be defined
// without issues
if siwa.private_key.is_none() {
if let Some(private_key_file) = siwa.private_key_file.take() {
let key = tokio::fs::read_to_string(private_key_file).await?;
siwa.private_key = Some(key);
}
}
let encoded = serde_json::to_vec(&siwa)?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does mean that it's possible to end up in the database with no private key if none are set. But I'm fine with that, SiWA is sufficiently niche anyway :)

@sandhose sandhose enabled auto-merge April 16, 2025 09:36
@sandhose sandhose merged commit 63a7c87 into element-hq:main Apr 16, 2025
20 checks passed
@sandhose sandhose changed the title Require auth key file for Apple Sign-In Allow using a separate key file when setting up 'Sign in with Apple' Apr 30, 2025
@sandhose sandhose added A-Configuration Related on what is configurable and how it can be configured T-Enhancement New feature of request labels Apr 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Configuration Related on what is configurable and how it can be configured T-Enhancement New feature of request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Apple SSO Provider Docs Incorrect/Incomplete

3 participants